Refactor node configs to remove local-only addresses from genesis #6629
Conversation
github-actions
bot
added
the
Type: Documentation
aschran
force-pushed
the
aschran/config
branch
3 times, most recently
from
ad0bb83
to
8915d8d
Compare
aschran
requested review from
huitseeker,
tharbert,
randall-Mysten and
ronny-mysten
as code owners
|
@tharbert are there configs in other repos that I'll need to update after this change to migrate |
There was a problem hiding this comment.
This generally looks good. I wonder, for now, if we want to always configure worker/primary address for our own worker/primary to be localhost:{port in the committee}.
I also want someone from narwhal to double check this, maybe @akichidis?
| narwhal_config::WorkerInfo { | ||
| name: NetworkPublicKey::from_bytes(validator.worker_key().as_ref()) | ||
| .expect("Can't get worker key"), | ||
| transactions: consensus_config.address.clone(), | ||
| worker_address, | ||
| }, |
There was a problem hiding this comment.
In a follow up PR maybe we should remove transactions from the narwhal WorkerInfo (or allow for it to be empty) since its unused in Sui and this value put here is just a placeholder for all entries other than our worker.
I would opt not to hardcode localhost, since I think that we don't want that to be the long-term default anyway? My understanding is this is just an escape hatch for partners with broken routing? |
|
Yeah i do much prefer this as an escape hatch. |
| .clone() | ||
| }; | ||
| let workers = [( | ||
| 0, // worker_id |
There was a problem hiding this comment.
Not a concern of this PR, but since we are here the worker_id should also be configurable in the future and not hardcoded - but we can refactor this when we deal with the story of spinning up multiple workers.
There was a problem hiding this comment.
Yea it doesn't work at all for >1 worker right now so I figured that should all be done at once
There was a problem hiding this comment.
Changes look good, thanks @aschran ! Could you please ensure that Narwhal benchmarks are also working fine after the changes? (since we have now removed the internal_worker_address ? )
Also very good point about infra changes that need to be made - let's make sure that those will be addressed as well.
There was a problem hiding this comment.
Thanks for this PR!
referencing a slack conversation
I think the difference is that genesis is global/public and the other parts of NodeConfig aren't necessarily so
If that is the case, maybe we can rename the things that are called genesis but actually just public to public here?
aschran
force-pushed
the
aschran/config
branch
from
8915d8d
to
073747e
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
For posterity: @tharbert said this should be fine to merge, the main thing that'll need updates is https://github.com/MystenLabs/sui-operations/tree/main/docker/config/generate/templates which he'll do before Monday PT. |
This removes the notion of internal address from the narwhal side, instead configuring that externally.
aschran
force-pushed
the
aschran/config
branch
from
073747e
to
d7d8bf8
Compare
This removes the notion of internal address from the narwhal side, instead configuring that externally.